refactor: dedup estimateNeurons and the configured-repo-set assembly - #10179
Merged
Conversation
estimateNeurons existed four times and extractAiText three, byte-identical copies that had already drifted: ai-review.ts exports a version taking a `calls` multiplier, while ai-chat-qa, ai-summaries and ai-intent-router each kept a private copy without it. That is not just duplication. `estimatedNeurons` is BOTH the pre-flight budget gate and the value recorded into ai_usage_events, which sumAiEstimatedNeuronsSince adds up as the shared daily neuron budget. ai-chat-qa retries its provider call on an empty completion, so a retry spent two calls and reported one -- hiding usage from the backstop whose job is to notice runaway usage, in the one direction that matters. Both helpers move to a leaf module both sides import. Deliberately not imported from ai-review.ts: that file is 3.6k lines, and three small services should not depend on the review engine to divide a number by four. The chat surface's import-isolation contract (#4595 req 10) still holds -- the new module reaches no write path -- and its test still passes. The pre-flight gate keeps the single-call estimate, which is all that is knowable before the first attempt. The RECORDED figure is now computed after the loop from the calls actually made, so the two no longer pretend to be the same number. `calls` floors at 1, so a miscount can never report zero spend for work that happened. Found by the maintainability audit (#10170). Closes #10169
…pies Assembling "which repos does a fleet-wide pass consider" was duplicated five times, byte-identical: three copies inside queue/processors.ts and one each in review/pr-reconciliation.ts and review/sweep-watchdog.ts. Every fleet-wide sweep needs the same answer and every one of them rebuilt it. Only the ASSEMBLY moves. What each caller does next -- resolving settings, requiring a real installation, applying its own eligibility rule -- genuinely differs and stays at the call site. Extracting further would push those differences into flag parameters, which is how a shared helper ends up worse than the duplication it replaced. The merge semantics were load-bearing and untested: the convergence list wins on name, but carries over a local row's installationId when one exists. Losing that would make an installed repo look uninstalled, and every caller that requires an installation would skip it -- a fleet-wide sweep quietly doing nothing. Now covered directly, including the case-insensitive de-duplication and the omitted-not-null installationId shape the call sites' spreads depend on. processors.ts drops from 16,913 to 16,886 lines. Small, but the point is the four remaining copies that can no longer drift. Refs #10170
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Contributor
|
Important 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏳ LoopOver is waiting…LoopOver has seen this pull request and is waiting on CI checks to finish before reviewing it. This comment will update once the review runs. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟨 Waiting |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10179 +/- ##
==========================================
- Coverage 92.21% 91.34% -0.88%
==========================================
Files 932 934 +2
Lines 114112 114072 -40
Branches 27565 27547 -18
==========================================
- Hits 105228 104194 -1034
- Misses 7583 8772 +1189
+ Partials 1301 1106 -195
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #10169 · Refs #10170
Two duplication findings from the maintainability audit, bundled because they are the same class and both are bounded.
1.
estimateNeurons— four copies, already drifted, under-reporting real spendai-review.tsexports the canonical version taking acallsmultiplier.ai-chat-qa,ai-summariesandai-intent-routereach kept a private copy without it.extractAiTextis duplicated across the same three, byte-identical.This is not just tidiness.
estimatedNeuronsis both the pre-flight budget gate and the value recorded intoai_usage_events, whichsumAiEstimatedNeuronsSincetotals as the shared daily neuron budget.ai-chat-qaretries its provider call on an empty completion:Two calls, one call's neurons recorded — hiding spend from the backstop whose job is to notice runaway spend, in the one direction that matters.
Fix. Both helpers move to a leaf module (
ai-usage-estimate.ts) all callers import. Deliberately not imported fromai-review.ts: that file is 3.6k lines and three small services should not depend on the review engine to divide a number by four. The chat surface's import-isolation contract (#4595 req 10) still holds — the new module reaches no write path — and its test still passes.The pre-flight gate keeps the single-call estimate, which is all that is knowable before the first attempt. The recorded figure is now computed after the loop from calls actually made, so the two stop pretending to be one number.
callsfloors at 1, so a miscount can never report zero spend for work that happened.2. Configured-repo-set assembly — five byte-identical copies
Three inside
processors.ts, one each inpr-reconciliation.tsandsweep-watchdog.ts. Every fleet-wide sweep needs "which repos are in scope" and each rebuilt it.Only the assembly moves. What each caller does next — resolve settings, require a real installation, apply its own eligibility rule — genuinely differs and stays at the call site. Extracting further would push those differences into flag parameters, which is how a shared helper ends up worse than the duplication it replaced.
The merge semantics were load-bearing and never directly tested: the convergence list wins on name but carries over a local row's
installationId. Losing that makes an installed repo look uninstalled, and every caller that requires an installation skips it — a fleet-wide sweep quietly doing nothing. Now covered, along with the case-insensitive de-duplication and the omitted-not-nullinstallationIdshape the call sites' spreads depend on.processors.ts: 16,913 → 16,886 lines. Small; the point is four copies that can no longer drift.Verification
typecheck,dead-exports,dead-source-files,import-specifiersclean.callsmultiplier (the drift being fixed)